Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graphql, Web: Add remotes tab, list, add, and delete remotes #313

Merged
merged 19 commits into from
Dec 4, 2024

Conversation

liuliu-dev
Copy link
Contributor

No description provided.

@liuliu-dev liuliu-dev changed the title Graphql, Web: Add remotes tab Graphql, Web: Add remotes tab and list remotes Dec 3, 2024
@liuliu-dev liuliu-dev requested a review from tbantle22 December 3, 2024 19:31
@liuliu-dev liuliu-dev changed the title Graphql, Web: Add remotes tab and list remotes Graphql, Web: Add remotes tab, list, add, and delete remotes Dec 3, 2024
Copy link
Collaborator

@tbantle22 tbantle22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Will pull and run the workbench to check out the UI when it's ready

Comment on lines 10 to 11
export type AddRemoteArgs = DBArgs & { remoteName: string; remoteUrl: string };
export type RemoteArgs = DBArgs & { remoteName: string };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type AddRemoteArgs = DBArgs & { remoteName: string; remoteUrl: string };
export type RemoteArgs = DBArgs & { remoteName: string };
export type RemoteArgs = DBArgs & { remoteName: string };
export type AddRemoteArgs = RemoteArgs & { remoteUrl: string };

}
}

function getRemoteListRes(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably belongs in model

loading,
} = useMutation({
hook: useAddRemoteMutation,
refetchQueries: refetchBranchQueries(props.params),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be refetchRemoteQueries?

};

const DatabaseRemotesPage: NextPage<Props> = ({ params }) => (
<Page title={`Add remote for ${params.databaseName}`} noIndex>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Page title={`Add remote for ${params.databaseName}`} noIndex>
<Page title={`New remote for ${params.databaseName}`} noIndex>

params: DatabaseParams;
};

const DatabaseRemotesPage: NextPage<Props> = ({ params }) => (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const DatabaseRemotesPage: NextPage<Props> = ({ params }) => (
const DatabaseNewRemotePage: NextPage<Props> = ({ params }) => (


export default function ForRemotes({ params, newRemote }: Props): JSX.Element {
const feature = newRemote ? "Creating remotes" : "Viewing Remotes";
console.log("ForRemotes", params, newRemote);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

};

export default function ForRemotes({ params, newRemote }: Props): JSX.Element {
const feature = newRemote ? "Creating remotes" : "Viewing Remotes";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const feature = newRemote ? "Creating remotes" : "Viewing Remotes";
const feature = newRemote ? "Creating remotes" : "Viewing remotes";

graphql-server/src/remotes/remote.resolver.ts Show resolved Hide resolved
Comment on lines 20 to 25
{remote.fetchSpecs?.map((fs, i) => (
<span key={fs}>
{fs}
{i < (remote.fetchSpecs?.length ?? 0) - 1 ? ", " : ""}
</span>
))}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just remote.fetchSpecs?.join(",") here? Does the span apply any styling?

const [offset, setOffset] = useState(data?.remotes.nextOffset);
const [lastOffset, setLastOffset] = useState<Maybe<number>>(undefined);

const refetch = async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere?

Comment on lines 146 to 149
let sel = em.createQueryBuilder().select("*").from("dolt_remotes", "");
sel = sel.offset(args.offset);

return sel.limit(ROW_LIMIT + 1).getRawMany();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the let here? It can all just be chained together?

)
}
`;

export const DELETE_REMOTE = gql`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should go in RemotesPage too

@liuliu-dev liuliu-dev merged commit 2ff83fc into main Dec 4, 2024
2 checks passed
@liuliu-dev liuliu-dev deleted the liuliu/remote-tab branch December 4, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants